[redis-rs][core] Move connection refresh to the background #2915

GilboaAWS · 2025-01-05T09:55:56Z

Issue link

This Pull Request is linked to issue (URL): [#2910]

Checklist

Before submitting the PR make sure the following are checked:

This Pull Request is related to one issue.
Commit message has a detailed description of what changed and why.
Tests are added or updated.
CHANGELOG.md and documentation files are updated.
Destination branch is correct - main or release
Create merge commit if merging release branch into main, squash otherwise.

ikolomi

This is a substantial change in the core mechanics. It must include tests that fail w/o this change to prove it necessity

nihohit · 2025-01-06T10:20:35Z

instead of splitting your state between multiple maps and leaving room for error in reading the wrong map, consider just use an enum representing each connection state (Usable, Reconnecting, etc.) in the main connection map.

See

valkey-glide/glide-core/src/client/reconnecting_connection.rs

Line 43 in ffc679a

enum ConnectionState {

for an example on how to do this.

barshaul

First comments (stopped before fn update_refreshed_connection).
Please see all inline comments and see how it behaves with cases where refresh connections is being called only with management/user/both. Also, maybe I haven't got to it yet - but the refresh task should be bounded to the lifetime of the clusterNode / address, so it would be cancelled when refresh_slots removes it from the topology

glide-core/redis-rs/redis/src/cluster_async/connections_container.rs

glide-core/redis-rs/redis/src/cluster_async/mod.rs

barshaul · 2025-01-06T13:01:42Z

glide-core/redis-rs/redis/src/cluster_async/mod.rs

@@ -1798,9 +1858,8 @@ where
        if !failed_connections.is_empty() {
            Self::refresh_connections(
                inner,
-                failed_connections,
+                failed_connections.into_iter().collect::<HashSet<String>>(),


it creates an unnecessary copy. instead you can change failed_connections to be a set to begin with

Signed-off-by: GilboaAWS <[email protected]>

This reverts commit 4e6535f. Signed-off-by: GilboaAWS <[email protected]>

…eturned the refresh_connection logic of sending the connection but without removing it from the connection_map Signed-off-by: GilboaAWS <[email protected]>

… the tokio refresh task Signed-off-by: GilboaAWS <[email protected]>

Signed-off-by: GilboaAWS <[email protected]>

…tion before redirecting to a random node Signed-off-by: GilboaAWS <[email protected]>

Signed-off-by: GilboaAWS <[email protected]>

…ot needed in rust where tokio runs the synced poll_flush to the end and only then let the refresh task to update the connection map Signed-off-by: GilboaAWS <[email protected]>

…sk itself. Signed-off-by: GilboaAWS <[email protected]>

barshaul

See comments inline

barshaul · 2025-02-09T15:35:31Z

glide-core/redis-rs/redis/src/cluster_async/mod.rs

+        trace!("refresh_and_update_connections: calling trigger_refresh_connection_tasks");
+        Self::trigger_refresh_connection_tasks(
+            inner.clone(),
+            addresses.clone(),


why do you clone it?

Because the rust compiler demands it.

barshaul · 2025-02-09T15:37:02Z

glide-core/redis-rs/redis/src/cluster_async/mod.rs

+            .read()
+            .expect(MUTEX_READ_ERR)
+            .refresh_conn_state
+            .collect_refresh_notifiers(&addresses);


can't trigger_refresh_connection_tasks return the notifiers?

then you won't need to clone the addresses

The cost of cloning the addresses isn't a problem, especially the fact it happens in the slow path.
I think it's more clear to collect them with a dedicated function where needed, as we can see it's being used only here.

barshaul · 2025-02-09T15:39:51Z

glide-core/redis-rs/redis/src/cluster_async/mod.rs

@@ -1371,62 +1374,147 @@ where
        }
    }

-    async fn refresh_connections(
+    // Creates refresh tasks, await on the tasks' notifier and the update the connection_container.


"...and the update the connection_container."
It doesn't update the connection container. the description of this function isn't clear - please fix it

You're 100% right.

barshaul · 2025-02-09T15:54:48Z

glide-core/redis-rs/redis/src/cluster_async/mod.rs

+        let futures: Vec<_> = refresh_task_notifiers
+            .iter()
+            .map(|notify| notify.notified())
+            .collect();
+        futures::future::join_all(futures).await;


I think you can skip the memory allocation you get from 'collect' with:
join_all(refresh_task_notifiers.iter().map(|notify| notify.notified())).await;

barshaul · 2025-02-09T16:04:16Z

glide-core/redis-rs/redis/src/cluster_async/mod.rs

+                .refresh_address_in_progress
+                .contains_key(&address)
+            {
+                info!("Skipping refresh for {}: already in progress", address);


Should be changed to debug! as we can get a lot of this log when a node will be down and we'll have reconnection retries

barshaul · 2025-02-10T13:32:54Z

glide-core/redis-rs/redis/src/cluster_async/mod.rs

                    RefreshConnectionType::AllConnections,
-                    None,
-                    core.glide_connection_options.clone(),
+                    false,


Because when sending RefreshConnectionType::AllConnections, there is no need to to reuse old connection...

barshaul · 2025-02-10T13:58:54Z

glide-core/redis-rs/redis/src/cluster_async/mod.rs

                    }
+                };
+
+                let mut conn_option = None;


let conn_option = if let Some(refresh_notifier) = reconnect_notifier { ... } else { None };

barshaul · 2025-02-10T14:02:04Z

glide-core/redis-rs/redis/src/cluster_async/mod.rs

-                    None,
+            ConnectionCheck::OnlyAddress(address) => {
+                // No connection for this address in the conn_map
+                Self::trigger_refresh_connection_tasks(


again - can trigger_refresh_connection_tasks simply return the notifiers?

then it would reduce a lot of logic (lines 2423 and above)

barshaul · 2025-02-10T14:03:20Z

glide-core/redis-rs/redis/src/cluster_async/mod.rs

-                .get_node()
+                .await;
+
+                let reconnect_notifier: Option<Arc<Notify>> = match core


nit: reduce redundant code lines:

let reconnect_notifier = core .conn_lock .read() .expect(MUTEX_READ_ERR) .refresh_conn_state .refresh_address_in_progress .get(&address).map(|refresh_task_state| { if let RefreshTaskStatus::Reconnecting(refresh_notifier) = &refresh_task_state.status { Some(refresh_notifier.get_notifier()) } else { None }});

ignore if you change trigger_refresh_connection_tasks to return the notifiers

barshaul · 2025-02-10T14:17:49Z

python/python/tests/test_auth.py

@@ -66,6 +66,7 @@ async def test_update_connection_password(
        assert value == b"test_value"
        await glide_client.update_connection_password(None)
        await kill_connections(management_client)
+        await asyncio.sleep(1)


We want to make sure the connections were already established.

Signed-off-by: GilboaAWS <[email protected]>

GilboaAWS requested a review from a team as a code owner January 5, 2025 09:55

GilboaAWS requested review from barshaul and ikolomi January 5, 2025 10:04

GilboaAWS force-pushed the reconnect_to_bg branch from 31cf02f to f850d99 Compare January 5, 2025 10:06

ikolomi requested changes Jan 5, 2025

View reviewed changes

GilboaAWS force-pushed the reconnect_to_bg branch from ffbd018 to 30b35f2 Compare January 6, 2025 11:16

barshaul reviewed Jan 6, 2025

View reviewed changes

GilboaAWS force-pushed the reconnect_to_bg branch 2 times, most recently from 40744cb to 4e6535f Compare January 7, 2025 09:54

GilboaAWS marked this pull request as draft January 7, 2025 11:48

GilboaAWS self-assigned this Jan 7, 2025

GilboaAWS force-pushed the reconnect_to_bg branch from 6ad1fd7 to 66a3e39 Compare January 16, 2025 16:02

GilboaAWS force-pushed the reconnect_to_bg branch 11 times, most recently from a8588c1 to f51b647 Compare February 9, 2025 10:09

GilboaAWS marked this pull request as ready for review February 9, 2025 10:48

GilboaAWS added 5 commits February 9, 2025 13:29

[redis-rs][core] Move connection refresh to the background

25d776b

Signed-off-by: GilboaAWS <[email protected]>

formating using lint

d0777d7

Signed-off-by: GilboaAWS <[email protected]>

Fixing bug - Refresh addresses started to take place

8fa7faa

Signed-off-by: GilboaAWS <[email protected]>

Bug fix

7fbf26a

Signed-off-by: GilboaAWS <[email protected]>

Fix update_passwd test in python

bd21d9e

Signed-off-by: GilboaAWS <[email protected]>

GilboaAWS added 13 commits February 9, 2025 13:29

Clear refresh data on reconnect to initial nodes

bcf7c42

Signed-off-by: GilboaAWS <[email protected]>

Revert "Fix update_passwd test in python"

8fcdb7e

This reverts commit 4e6535f. Signed-off-by: GilboaAWS <[email protected]>

1. Replaced DashMap to HashMap 2. Moved refresh data to a struct 3. R…

c6fa9c3

…eturned the refresh_connection logic of sending the connection but without removing it from the connection_map Signed-off-by: GilboaAWS <[email protected]>

Adding address to be removed from the refresh_connection instead from…

6e0f8b5

… the tokio refresh task Signed-off-by: GilboaAWS <[email protected]>

lint fixes

60cbc37

Signed-off-by: GilboaAWS <[email protected]>

Add refresh task status to indicate if the task is running too long

e644eb4

Signed-off-by: GilboaAWS <[email protected]>

Updated get_connection for SpecificNode to await on refreshing connec…

48122c8

…tion before redirecting to a random node Signed-off-by: GilboaAWS <[email protected]>

Fix slow path reconnect process to call update on the connection map

5ffe76e

Signed-off-by: GilboaAWS <[email protected]>

fix deadlock

e8d8913

Signed-off-by: GilboaAWS <[email protected]>

fix trigger refresh order

5cdfa2e

Signed-off-by: GilboaAWS <[email protected]>

fix lint for redis-rs

4d44ba1

Signed-off-by: GilboaAWS <[email protected]>

Removing the sync point inside the poll_flush as we understand it's n…

9c8543e

…ot needed in rust where tokio runs the synced poll_flush to the end and only then let the refresh task to update the connection map Signed-off-by: GilboaAWS <[email protected]>

Some edits as we moved to update the connection map from the tokio ta…

4799974

…sk itself. Signed-off-by: GilboaAWS <[email protected]>

GilboaAWS force-pushed the reconnect_to_bg branch from f51b647 to 4799974 Compare February 9, 2025 11:29

barshaul requested changes Feb 10, 2025

View reviewed changes

Addressing comments

2fd60f4

Signed-off-by: GilboaAWS <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[redis-rs][core] Move connection refresh to the background #2915

[redis-rs][core] Move connection refresh to the background #2915

GilboaAWS commented Jan 5, 2025

ikolomi left a comment

nihohit commented Jan 6, 2025 •

edited

Loading

barshaul left a comment •

edited

Loading

barshaul Jan 6, 2025

barshaul left a comment

barshaul Feb 9, 2025

GilboaAWS Feb 11, 2025

barshaul Feb 9, 2025

barshaul Feb 9, 2025

GilboaAWS Feb 11, 2025

barshaul Feb 9, 2025

GilboaAWS Feb 11, 2025

barshaul Feb 9, 2025

barshaul Feb 9, 2025

barshaul Feb 10, 2025

GilboaAWS Feb 11, 2025

barshaul Feb 10, 2025

barshaul Feb 10, 2025

barshaul Feb 10, 2025

barshaul Feb 10, 2025

barshaul Feb 10, 2025

barshaul Feb 10, 2025

GilboaAWS Feb 11, 2025

[redis-rs][core] Move connection refresh to the background #2915

Are you sure you want to change the base?

[redis-rs][core] Move connection refresh to the background #2915

Conversation

GilboaAWS commented Jan 5, 2025

Issue link

Checklist

ikolomi left a comment

Choose a reason for hiding this comment

nihohit commented Jan 6, 2025 • edited Loading

barshaul left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

barshaul left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nihohit commented Jan 6, 2025 •

edited

Loading

barshaul left a comment •

edited

Loading